packages/cipherstash-proxy-integration/src/multitenant/ore_order.rs (3)

30-48: Duplicated utility function.

interleaved_indices is duplicated verbatim from map_ore_index_order.rs. Consider extracting it to the crate::common module to avoid duplication.

♻️ Extract to common module

In crate::common:

/// Returns indices in zigzag order so insertion is never accidentally sorted.
/// For len=5: [4, 0, 3, 1, 2]
pub fn interleaved_indices(len: usize) -> Vec<usize> {
    // ... implementation
}

Then import in both test files:

use crate::common::interleaved_indices;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cipherstash-proxy-integration/src/multitenant/ore_order.rs` around
lines 30 - 48, The function interleaved_indices is duplicated; extract it into a
shared module (e.g., crate::common) as a public function pub fn
interleaved_indices(len: usize) -> Vec<usize> with the same implementation, then
replace the local definitions in multitenant::ore_order.rs and
map_ore_index_order.rs by importing use crate::common::interleaved_indices; and
remove the duplicate implementations so both files call the single shared
function.

---

16-22: SQL injection risk in connect_as_tenant with string interpolation.

The keyset_id is interpolated directly into the SQL string. While the value comes from environment variables (which should be UUIDs), the proxy validates the UUID format server-side (per maybe_set_keyset_id in context snippet 1). However, using parameterized queries would be more defensive.

That said, since this is test code and the values are from controlled environment variables, the risk is minimal.

♻️ Optional: Use EXECUTE for parameterized SET

If you want to be extra defensive, you could validate the UUID format before interpolation:

 async fn connect_as_tenant(keyset_id: &str) -> tokio_postgres::Client {
     let client = connect_with_tls(PROXY).await;
+    // Validate UUID format before interpolation
+    uuid::Uuid::parse_str(keyset_id)
+        .expect("keyset_id must be a valid UUID");
     let sql = format!("SET CIPHERSTASH.KEYSET_ID = '{keyset_id}'");
     client.execute(&sql, &[]).await.unwrap();
     client
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cipherstash-proxy-integration/src/multitenant/ore_order.rs` around
lines 16 - 22, The connect_as_tenant function currently interpolates keyset_id
into the SQL string which can lead to injection; instead, validate the keyset_id
as a UUID (e.g., use uuid::Uuid::parse_str) and/or issue the SET using a
parameterized execution via tokio_postgres (reference connect_as_tenant,
connect_with_tls, PROXY and the "SET CIPHERSTASH.KEYSET_ID" statement) so the
value is either rejected if not a valid UUID or passed as a query parameter
rather than concatenated into the SQL.

---

50-436: Test helpers are largely duplicated from map_ore_index_order.rs.

The helper functions (ore_order_text, ore_order_generic, etc.) duplicate logic from map_ore_index_order.rs. The key difference is that these accept a &Client parameter while the original tests create their own client.

This duplication is understandable given the different test structures, but if maintainability becomes a concern, you could refactor map_ore_index_order.rs to also use client-accepting helpers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cipherstash-proxy-integration/src/multitenant/ore_order.rs` around
lines 50 - 436, Several helper tests (ore_order_text, ore_order_text_desc,
ore_order_nulls_last_by_default, ore_order_nulls_first,
ore_order_qualified_column, ore_order_qualified_column_with_alias,
ore_order_no_eql_column_in_select_projection, ore_order_plaintext_column,
ore_order_plaintext_and_eql_columns, ore_order_simple_protocol,
ore_order_generic) are duplicated from map_ore_index_order.rs; extract the
shared implementations into a single test helper module that accepts
&tokio_postgres::Client (e.g., tests::ore_order_helpers) and move the bodies of
these functions there, then replace the duplicates in both files with calls to
the new helpers (keep the same function names/signatures so call sites only need
imports), update imports/visibility, and remove the duplicated definitions to
avoid code drift.

packages/cipherstash-proxy-integration/src/ore_order_helpers.rs (1)

368-394: Make sort direction type-safe in ore_order_generic.

Using a free-form &str means only exact "DESC" reverses expected; other variants silently behave like ASC in assertions. A small enum avoids this class of mistakes.

♻️ Proposed refactor

+pub enum SortDirection {
+    Asc,
+    Desc,
+}
+
+impl SortDirection {
+    fn as_sql(&self) -> &'static str {
+        match self {
+            SortDirection::Asc => "ASC",
+            SortDirection::Desc => "DESC",
+        }
+    }
+}
+
 pub async fn ore_order_generic<T>(
     client: &tokio_postgres::Client,
     col_name: &str,
     values: Vec<T>,
-    direction: &str,
+    direction: SortDirection,
 ) where
     for<'a> T: Clone + PartialEq + ToSql + Sync + FromSql<'a> + PartialOrd + Debug,
 {
@@
-    let select_sql = format!("SELECT {col_name} FROM encrypted ORDER BY {col_name} {direction}");
+    let select_sql = format!(
+        "SELECT {col_name} FROM encrypted ORDER BY {col_name} {}",
+        direction.as_sql()
+    );
@@
-    let expected: Vec<T> = if direction == "DESC" {
-        values.into_iter().rev().collect()
-    } else {
-        values
-    };
+    let expected: Vec<T> = match direction {
+        SortDirection::Desc => values.into_iter().rev().collect(),
+        SortDirection::Asc => values,
+    };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cipherstash-proxy-integration/src/ore_order_helpers.rs` around lines
368 - 394, Change the loose &str "direction" in ore_order_generic to a small
enum (e.g., enum SortDirection { Asc, Desc }) and update the function signature
and call sites to accept that enum; use a match on SortDirection to build the
ORDER BY clause string for select_sql and to decide whether to reverse expected
(match SortDirection::Desc => values.into_iter().rev().collect(),
SortDirection::Asc => values). Update any places constructing the SQL string
(the select_sql format) to convert the enum to the correct SQL token and keep
the rest of ore_order_generic (inserts, query, row extraction) unchanged.

packages/cipherstash-proxy-integration/src/map_ore_index_order.rs (1)

104-220: Consider centralizing shared numeric fixtures.

The same test value vectors are repeated across this file and packages/cipherstash-proxy-integration/src/multitenant/ore_order.rs. Extracting shared constants reduces drift and future update misses.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cipherstash-proxy-integration/src/map_ore_index_order.rs` around
lines 104 - 220, Tests repeat identical numeric vectors across multiple test
functions (e.g., map_ore_order_int2, map_ore_order_int4, map_ore_order_int8,
map_ore_order_float8 and their _desc variants) and across another test file;
extract these fixtures into shared constants in a single place (for example add
pub const/ static arrays in ore_order_helpers.rs or a new test_fixtures module)
and replace the inline Vec literals with references to those constants (e.g.,
use ENCRYPTED_INT2_VALUES, ENCRYPTED_INT4_VALUES, ENCRYPTED_INT8_VALUES,
ENCRYPTED_FLOAT8_VALUES) so both this file and the multitenant/ore_order.rs can
import and reuse them.

packages/cipherstash-proxy-integration/src/ore_order_helpers.rs (1)

392-403: Constrain dynamic column SQL to an allowlist type.

Line 392 and Line 402 interpolate col_name directly into SQL. It is currently safe at callsites, but this helper is pub and easy to misuse later. Prefer a typed column enum with fixed SQL identifiers.

Refactor sketch

+pub enum OreOrderColumn {
+    EncryptedInt2,
+    EncryptedInt4,
+    EncryptedInt8,
+    EncryptedFloat8,
+}
+
+impl OreOrderColumn {
+    fn as_sql_ident(&self) -> &'static str {
+        match self {
+            OreOrderColumn::EncryptedInt2 => "encrypted_int2",
+            OreOrderColumn::EncryptedInt4 => "encrypted_int4",
+            OreOrderColumn::EncryptedInt8 => "encrypted_int8",
+            OreOrderColumn::EncryptedFloat8 => "encrypted_float8",
+        }
+    }
+}
+
 pub async fn ore_order_generic<T>(
     client: &tokio_postgres::Client,
-    col_name: &str,
+    col: OreOrderColumn,
     values: Vec<T>,
     direction: SortDirection,
 ) where
     for<'a> T: Clone + PartialEq + ToSql + Sync + FromSql<'a> + PartialOrd + Debug,
 {
+    let col_name = col.as_sql_ident();
     let insert_sql = format!("INSERT INTO encrypted (id, {col_name}) VALUES ($1, $2)");
     ...
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cipherstash-proxy-integration/src/ore_order_helpers.rs` around lines
392 - 403, The code interpolates col_name directly into SQL (used in insert_sql
and select_sql) which is unsafe for a public helper; replace the dynamic string
with a typed Column enum (e.g., pub enum Column { Foo, Bar, ... }) and give it a
method sql_name(&self) -> &'static str that returns the fixed identifier for
each variant; change this helper's signature to accept Column instead of a
string, call col.sql_name() when building insert_sql and select_sql, and update
all callsites (and any tests) to pass the enum; keep direction.as_sql() as-is
and do not interpolate any other unchecked identifiers.

packages/cipherstash-proxy-integration/src/multitenant/ore_order.rs (1)

21-22: Prefer parameterized session config over formatted SQL.

Line 21 and Line 22 build a SQL string for SET. Even with UUID validation, using set_config(..., $1, ...) avoids manual SQL construction and keeps this helper safer to evolve.

Refactor sketch

-        let sql = format!("SET CIPHERSTASH.KEYSET_ID = '{keyset_id}'");
-        client.execute(&sql, &[]).await.unwrap();
+        client
+            .execute(
+                "SELECT set_config('cipherstash.keyset_id', $1, false)",
+                &[&keyset_id],
+            )
+            .await
+            .unwrap();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cipherstash-proxy-integration/src/multitenant/ore_order.rs` around
lines 21 - 22, Replace the formatted SET statement that builds sql with a
parameterized call to set_config to avoid manual SQL construction: instead of
creating sql via format!("SET CIPHERSTASH.KEYSET_ID = '{keyset_id}'") and
calling client.execute(&sql, &[]).await.unwrap(), call client.execute with a
parameterized query that invokes set_config('CIPHERSTASH.KEYSET_ID', $1, false)
(or equivalent) and pass keyset_id as the parameter; keep references to the
existing keyset_id variable and the client.execute invocation so the change is
localized to that helper.